[ENG-8064] Notification Refactor Phase 2 #11322

cslzchen · 2025-09-18T15:24:50Z

Purpose

TBD

Changes

TBD

QA Notes

TBD

Documentation

TBD

Side Effects

TBD

Ticket

https://openscience.atlassian.net/browse/ENG-8064

--------- Co-authored-by: John Tordoff <Johnetordoff@users.noreply.github.com> Co-authored-by: Ostap Zherebetskyi <ozherebetskyi@exoft.net> Co-authored-by: Bohdan Odintsov <bodintsov@exoft.net> Co-authored-by: John Tordoff <>

* Fix moderator digest * Fix unit tests

…ion (#11324) * Fix issue with user confirmation/merger creating subscription * Add docstrings

* Add No Login to notification template and add to tests

…tion-refactor-p2-s * Conflicts have not been resolved in this merge since we want to track how we fixed the conflicts due to complexity * api/nodes/serializers.py * osf/models/user.py * tests/test_auth.py * tests/test_webtests.py * website/templates/node_request_institutional_access_request.html.mako

…tically

…plate-with-naming [ENG-8988] Fix/notification contrib template with naming

[ENG-8997] Turn on script tests

opaduchak · 2025-10-15T11:14:43Z

addons/boa/tests/test_tasks.py

    def tearDown(self):
        super().tearDown()


opaduchak · 2025-10-15T11:15:45Z

addons/osfstorage/tests/test_models.py

+        with capture_notifications():
+            fork = node.fork_node(auth_obj)


Should not we check for captured notifications?

opaduchak · 2025-10-15T13:00:57Z

api/providers/tasks.py

-            message = f'Failed to send registration bulk upload outcome email due to invalid ' \
-                      f'upload state: [upload={upload.id}, state={upload.state.name}]'
-            logger.error(message)
-            sentry.log_message(message)


Do we really not want this in sentry?

opaduchak · 2025-10-15T13:14:18Z

api_tests/draft_registrations/views/test_draft_registration_contributor_list.py

            res = app.post_json_api(url, payload, auth=user.auth)
        assert contributor_added in mock_signal.signals_sent()
        assert res.status_code == 201
-        assert mock_send_grid.call_count == 0


I suppose we need to verify that emails were not sent

opaduchak · 2025-10-15T13:16:02Z

api_tests/institutions/views/test_institution_auth.py

        username, fullname, password = 'user_inactive@osf.edu', 'Foo Bar', 'FuAsKeEr'
        user = make_user(username, fullname)
-        user.set_password(password)
+        with capture_notifications() as mock_signals:


Suggested change

with capture_notifications() as mock_signals:

with capture_notifications():

opaduchak · 2025-10-17T13:13:38Z

tests/test_webtests.py

@@ -467,631 +462,6 @@ def test_wiki_url(self):
            assert self._url_to_body(self.wiki.deep_url) == self._url_to_body(self.wiki.url)


-@pytest.mark.enable_bookmark_creation


Please confirm this to be intended removal

opaduchak · 2025-10-17T13:14:57Z

tests/utils.py

 @contextlib.contextmanager
 def run_celery_tasks():
    yield
    celery_teardown_request()
+
+import re, html as html_lib, difflib


move imports to the start of the file

opaduchak · 2025-10-17T13:16:17Z

tests/utils.py

+def _safe_user_id(u: Any) -> Optional[str]:
+    """Normalize user object to stable identifier."""
+    if u is None:
+        return None
+    for attr in ('_id', 'id', 'guids', 'guid', 'pk'):
+        if hasattr(u, attr):
+            try:
+                val = getattr(u, attr)
+                if hasattr(val, 'first'):
+                    g = val.first()
+                    if g and hasattr(g, '_id'):
+                        return g._id
+                if isinstance(val, (str, int)):
+                    return str(val)
+            except Exception:
+                pass
+    return f'obj:{id(u)}'


please fix variable names

opaduchak · 2025-10-17T13:16:30Z

tests/utils.py

+def _safe_obj_id(o: Any) -> Optional[str]:
+    """Normalize object to stable identifier."""
+    if o is None:
+        return None
+    for attr in ('_id', 'id', 'guid', 'guids', 'pk'):
+        if hasattr(o, attr):
+            try:
+                val = getattr(o, attr)
+                if hasattr(val, 'first'):
+                    g = val.first()
+                    if g and hasattr(g, '_id'):
+                        return g._id
+                if isinstance(val, (str, int)):
+                    return str(val)
+            except Exception:
+                pass
+    return f'obj:{id(o)}'


please fix variable names

opaduchak · 2025-10-17T13:17:46Z

website/conferences/views.py

@@ -125,36 +82,6 @@ def add_poster_by_email(conference, message):

    utils.upload_attachments(user, node, message.attachments)

-    download_url = node.web_url_for(


are these removals intended?

[ENG-8996] Fix moderator added template

opaduchak

Several leftover questions:
General opinion on templates:
We need to look through each one, identify redundant variables in event context and remove them. Also we need to verify that all required variables are indeed passed to templates.

This review contains only several topmost templates, therefore there supposedly are many more issues than comments

opaduchak · 2025-10-21T10:46:35Z

website/project/views/comment.py

@@ -15,6 +15,7 @@

 @file_updated.connect
 def update_file_guid_referent(self, target, event_type, payload, user=None):
+    event_type = payload['action']


do we even need event_type parameter in this case?
I guess we should remove it or at least make it optional

opaduchak · 2025-10-21T11:11:28Z

osf/utils/machines.py

        context['contributors_url'] = f'{self.machineable.target.absolute_url}contributors/'
        context['project_settings_url'] = f'{self.machineable.target.absolute_url}settings/'


These are not needed, they are not present in template

opaduchak · 2025-10-21T11:12:37Z

website/templates/add_sso_email_osf4i.html.mako

This template is not used

opaduchak · 2025-10-21T11:14:15Z

website/templates/addons_boa_job_complete.html.mako

@@ -7,7 +7,7 @@
 </tr>
 <tr>
  <td style="border-collapse: collapse;">
-      Hello ${fullname},<br>
+      Hello ${user_fullname},<br>


during invocation, fullname is passed instead of user_fullname

opaduchak · 2025-10-21T11:17:51Z

website/templates/archive_copy_error_desk.html.mako

results are not being passed during invocation

opaduchak · 2025-10-21T11:19:51Z

website/archiver/utils.py

+        event_context={
+            'domain': settings.DOMAIN,
+            'user_fullname': user.fullname,
+            'user__id': user._id,
+            'src__id': src._id,
+            'src_url': src.url,
+            'src_title': src.title,
+            'results': results,
+            'url': url,
+            'can_change_preferences': False,
+        }


We need only src_title here

[ENG-9005] Fix/update v2 subscriptions

cslzchen and others added 17 commits September 18, 2025 11:22

Squash Merge Notification Refactor Phase 2 (#11321)

0ae75d0

--------- Co-authored-by: John Tordoff <Johnetordoff@users.noreply.github.com> Co-authored-by: Ostap Zherebetskyi <ozherebetskyi@exoft.net> Co-authored-by: Bohdan Odintsov <bodintsov@exoft.net> Co-authored-by: John Tordoff <>

[ENG-8996] Fix moderator digest (#11319)

5e29285

* Fix moderator digest * Fix unit tests

[ENG-8978] Fix issue with user confirmation/merger creating subscript…

ffa65ec

…ion (#11324) * Fix issue with user confirmation/merger creating subscription * Add docstrings

[ENG-8995] Fix issue with no login not being sent (#11326)

d24cb2c

* Add No Login to notification template and add to tests

Update v2 subscription API for Ag and new data

cf68080

fix issues with the API backward compatibility

caca24c

Fix merge conflicts as a result of merge commit 7975650

6c987b9

fix template language

8d55887

reorganize templates to deal with conditional statement better gramma…

cc84096

…tically

Fix templates

cce5b22

Merge pull request #11370 from bodintsov/fix/notification-contrib-tem…

491b828

…plate-with-naming [ENG-8988] Fix/notification contrib template with naming

fix unit tests

469ea23

Turn on script tests

7969612

fix merge conflict issues

b48fc7e

Merge pull request #11372 from Ostap-Zherebetskyi/fix/script_tests

25ac360

[ENG-8997] Turn on script tests

fix email template mismatch

38d6315

opaduchak suggested changes Oct 17, 2025

View reviewed changes

Merge pull request #11328 from Johnetordoff/fix-moderator-added-template

fd453ec

[ENG-8996] Fix moderator added template

opaduchak reviewed Oct 21, 2025

View reviewed changes

Merge pull request #11329 from Johnetordoff/fix/update-v2-subscriptions

0b7fe79

[ENG-9005] Fix/update v2 subscriptions

		with capture_notifications():
		fork = node.fork_node(auth_obj)

	with capture_notifications() as mock_signals:
	with capture_notifications():

		@@ -467,631 +462,6 @@ def test_wiki_url(self):
		assert self._url_to_body(self.wiki.deep_url) == self._url_to_body(self.wiki.url)


		@pytest.mark.enable_bookmark_creation

		@@ -125,36 +82,6 @@ def add_poster_by_email(conference, message):

		utils.upload_attachments(user, node, message.attachments)

		download_url = node.web_url_for(

		context['contributors_url'] = f'{self.machineable.target.absolute_url}contributors/'
		context['project_settings_url'] = f'{self.machineable.target.absolute_url}settings/'

[ENG-8064] Notification Refactor Phase 2 #11322

Are you sure you want to change the base?

[ENG-8064] Notification Refactor Phase 2 #11322

Uh oh!

Conversation

cslzchen commented Sep 18, 2025

Purpose

Changes

QA Notes

Documentation

Side Effects

Ticket

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opaduchak left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

opaduchak left a comment •

edited

Loading